-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Added configuration and Integration test to restrict public template … #4774
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
engine/components-api/src/main/java/com/cloud/template/TemplateManager.java
Outdated
Show resolved
Hide resolved
|
@blueorangutan package |
|
@harikrishna-patnala a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2890 |
|
@soreana, does this have any impact on upgrades, existing templates in the environment might have already shared and used across domains. Does a domain lose viewing or accessing the templates which are already used when the configuration parameter is set to true. |
|
Packaging result: ✖️ centos7 ✖️ centos8 ✔️ debian. SL-JID 59 |
|
Packaging result: ✔️ centos7 ✖️ centos8 ✔️ debian. SL-JID 63 |
|
@harikrishna-patnala I didn't noticed any issue. Here is what I tested today.
Let me know if you have other concerns. Btw, while I tried to reinstall the Without restriction (global setting sets to false)With restriction (global setting sets to true) |
|
@soreana I have raised the question exactly like Reinstall VM you mentioned above, loosing access to the template which are already in use. May be keeping the configuration scope to Domain level will give flexibility in allowing access in these cases. Let's hear from others too. Thanks. |
|
Not that I want to intrude on your discussion @soreana @harikrishna-patnala , but would it make sense to add a scope to the filter: all, featured, self, selfexecutable, sharedexecutable, executable, community, domain |
|
@DaanHoogland It is nice feature. But it will not address access right issue fixed in this pr. |
|
@harikrishna-patnala In regards to your As a result, with or without these changes, the test2 user might lose access to the template. It is an inherent risk in using community templates. :D Reinstall VM before making Debian template privateReinstall VM after making Debian template private |
|
Thanks @soreana for testing further. If you can add scope to domain that will be great. |
|
@harikrishna-patnala I updated the PR. I migrated the setting to the domain level. It is a limited code change required to do so :D. I tested that with subdomains as well. More details: If you set If you set Let me know if you have other concerns. P.S: I'm updating the test cases. |
|
Hi @soreana looks like the PR has a conflict, can you please resolve it? |
|
domain scope change looks good @soreana, please resolve the conflicts on the PR. |
|
I fixed the conflict. |
|
No problem, thanks @soreana |
|
@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 528 |
|
@blueorangutan test |
|
@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
DaanHoogland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks generally good, needs testing and some remarks on the integration test
test/integration/component/test_template_access_across_domains.py
Outdated
Show resolved
Hide resolved
test/integration/component/test_template_access_across_domains.py
Outdated
Show resolved
Hide resolved
test/integration/component/test_template_access_across_domains.py
Outdated
Show resolved
Hide resolved
test/integration/component/test_template_access_across_domains.py
Outdated
Show resolved
Hide resolved
test/integration/component/test_template_access_across_domains.py
Outdated
Show resolved
Hide resolved
|
Trillian test result (tid-1246)
|
|
@soreana can you address the open comments on the marvin tests? |
4db539b to
b9ec1de
Compare
|
Packaging result: ✖️ el7 ✖️ el8 ✖️ debian ✖️ suse15. SL-JID 3244 |
|
@blueorangutan package |
|
@rohityadavcloud a Jenkins job has been kicked to build packages. It will be bundled with SystemVM template(s). I'll keep you posted as I make progress. |
|
Packaging result: ✔️ el7 ✖️ el8 ✖️ debian ✖️ suse15. SL-JID 3251 |
|
@blueorangutan package |
|
@nvazquez a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 3255 |
|
@blueorangutan test |
|
@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Manually covered with the following tests, and verified the public templates access restricted to other domains when the global or the respective domain's config "share.public.templates.with.other.domains" is set to false.
Tests with global level setting: Tests with domain level setting: |
|
Hi @soreana all looking good except travis that is still failing in all these cases: |
| Function to update the global setting "restrict.public.access.to.templates" for domain | ||
| """ | ||
| update_configuration_cmd = updateConfiguration.updateConfigurationCmd() | ||
| update_configuration_cmd.name = "restrict.public.template.access.to.domain" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soreana
since you have changed the name of global configuration to share.public.templates.with.other.domains, can you change the name in component test ? (maybe the value as well)
The test needs to be updated to use the new configuration name
|
Found UI changes, kicking a new UI QA build |
|
Trillian test result (tid-3958)
|
PR Coverage Report
|
…PR: apache#4774 in commit e94c1e2. Had to make a few changes to support 4.11 and cleanup some of the quirks.
* Pulling in the relevant parts of the global settings version of this PR: apache#4774 in commit e94c1e2. Had to make a few changes to support 4.11 and cleanup some of the quirks. * Making the 'allow.public.user.templates' global setting allow domain and resource admins to still upload public templates. Previously it was limited to just root admins. * One additional template filter type that includes public templates * Fixing permissions bug where there weren't permissions to templates in child domains * Updating comments based on code review * Was previously allowing public, but not featured for domain admins. Added featured for domain admins. Also limited public to only when the new setting is enabled.




Description
As a cloud provider, we don't want our customers to see other templates. This pr limits template access to the domain.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
How Has This Been Tested?
To test this feature, I created two domains named Test0, Test1 and Test2, each with their respective domain admins (test0, test1 and test2).
I used cloudmonkey command to list different combination of templateFilters ( all, featured, self, selfexecutable, sharedexecutable, executable and community ) and accounts ( admin, test0, test1 and test2 ).
Pre configuration:
Test case one.
share.public.templatesto false for every domainTest case two
share.public.templateslike the following table.test0account. You should see combination of theU20, U0, U2with differenttemplatefilterbut notU1Test case three
share.public.templateslike the test case twotest0with template id. You should be ablet to see theU20, U0, U2but empty result if you useU1id.I wrote this script to test this pr, you can find it in the following link. You need the
cmkcommand and you should putadmin,test1, andtest2users info in the cmk configuration file. How to run this?